ENH: Adds "Min" Method to ProjectionPlot #3940

rjfarber · 2022-05-25T17:06:43Z

Example/which one???

PR Summary

This change is not required. But I found that I wanted to take a minimum projection plot and maximum is already implemented, so why not?

PR Checklist

[x ] New features are documented, with docstrings and narrative docs
Adds a test for any bugs fixed. Adds tests for new features.

Okay :)

…ue #987654321

…he cookbook and a mwe as 'simple_projection_methods.py' - and it looks like it's working! At least partially :) Addresses Issue #987654321

welcome · 2022-05-25T17:06:45Z

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

matthewturk · 2022-05-25T17:11:36Z

I love it! Thanks, @rjfarber !

neutrinoceros

Quick comment. Note that you also need to somehow conform to code style standards. Our docs (dev guide, code styling) stipulates how.

doc/source/cookbook/simple_projection_methods.py

matthewturk · 2022-05-25T19:57:36Z

pre-commit.ci autofix

matthewturk · 2022-05-25T19:57:57Z

@rjfarber I think it needs to be turned on first, but I've asked the bot to do it for you. I think you'll need to pull first before you make any more changes.

for more information, see https://pre-commit.ci

neutrinoceros

More remarks. If you wish to add a deprecation please see the dev guide for how to do it.

yt/utilities/lib/quad_tree.pyx

doc/source/cookbook/simple_projection_methods.py

yt/data_objects/construction_data_containers.py

…k. Address Issue #987654321

… 'mip' in construction_data_containers. I'm not sure now if I should've done that to clean or left it in there to be safe...

neutrinoceros · 2022-05-26T09:04:41Z

yt/data_objects/construction_data_containers.py

+                since="4.2",
+                removal="5.2",


since refers to the first release this deprecation lands on, so it should be 4.1 (not released yet)

removal is optional and shouldn't be used in general (it's mostly meant for stuff that was deprecated long ago, and that we know we can actually remove soon)

another important thing is that you should make sure that stack_level is correctly set so that users will see the actual line they typed in the warning rather than some random line internal to yt that they know nothing about.

Okay, I updated since to 4.1 and removed removal.

Regarding stack_level I tried grep -rin "stack_level" from the top level but it didn't return anything. So then I looked at a few other deprecation warnings and it doesn't look like they include it either. So I guess this is a larger issue (?) For example, the deprecation warning for style instead of method in ProjectionPlot is what I copied from...

My mistake, the argument is called stacklevel. I just tested it and here I think it should be set to stacklevel=4 so we see the line where the YTProj object is created.
It would also be helpful to have a separate deprecation warning closer to users, directly in AxisAlignedProjectionPlot.__init__, and make sure we don't trigger both warnings at the same time (meaning, AxisAlignedProjectionPlot should also be able to replace "mip" with "max" so the YTProj object created internally doesn't need to emit a second warning)

It doesn't look like stacklevel is being used anywhere so far:

(yt-fork-env) ryanjsfx@MB-145 yt-fork % grep -rin stacklevel . ./yt/_maintenance/deprecation.py:19: msg: str, *, since: str, removal: Optional[str] = None, stacklevel: int = 3 ./yt/_maintenance/deprecation.py:50: warnings.warn(msg, VisibleDeprecationWarning, stacklevel=stacklevel)

but okay I set stacklevel=4 (will commit after I get the cookbook working).

Theoretically, should that also be true of the deprecation warning for the use of 'style' instead of 'method' keyword arguments for ProjectionPlot?

====
Regarding AxisAlignedProjectPlot.init triggering the deprecation warning, should that also be true for the style vs method usage then? If so, maybe beyond the scope of this PR...

It doesn't look like stacklevel is being used anywhere so far:

this is indeed a problem we'll want to fix globally. I wasn't aware of this, thank you for calling it out !

Theoretically, should that also be true of the deprecation warning for the use of 'style' instead of 'method' keyword arguments for ProjectionPlot?

yes !

Regarding AxisAlignedProjectPlot.init triggering the deprecation warning, should that also be true for the style vs method usage then? If so, maybe beyond the scope of this PR.

Agreed. You don't need to fix these in your PR, but thanks a lot for raising this concern, I'll dedicate a PR to fixing this globally ! However my point about adding a depr to AxisAlignedProjectionPlot still stands :)

Okay I moved the deprecation warning to AxisAlignedProjectionPlot (which will propagate here once I get pre-commit happy...)

Is class YTProj guaranteed to inherit from the axis-aligned version of ProjectionPlot though? If not, the deprecation won't trigger.

Okay I moved the deprecation warning to AxisAlignedProjectionPlot (which will propagate here once I get pre-commit happy...)

There's a slight misunderstanding. I meant that we want two deprecation warnings to exist, not move the one you already had.

Is class YTProj guaranteed to inherit from the axis-aligned version of ProjectionPlot though? If not, the deprecation won't trigger.

So YTProj doesn't actually inherit anything from these classes, but I think I know what you mean, and indeed the deprecation should also be seen for OffAxisProjectionPlot so it's best to put it in the base ProjectionPlot class.

Sooooo class ProjectionPlot(NormalPlot) only has a __new__ method (no __init__) and it's not very clear to me if it takes in method (maybe as part of *args (?))

I put in the deprecation warning but doesn't look like it fits aesthetically at least 🤔

Nerp it yields an UnboundLocalError method referenced before assignment

Yeah this is delicate. Maybe just don't worry about it, I can do it myself once the rest of the PR is ready :)

yt/data_objects/construction_data_containers.py

… it was intended to have. Addresses issue #987654321

doc/source/cookbook/simple_projection_methods.py

custom file name Co-authored-by: Clément Robert <cr52@protonmail.com>

yt/data_objects/construction_data_containers.py

neutrinoceros

note that there a many cookbook example using method="mip", you'll want to update them as well so they don't trigger deprecation warnings

…t of little commits since I made a lot of changes and just lost them all :*( Addresses Issue #987654321

…projection method call

rjfarber · 2022-05-26T12:54:50Z

I only noticed one 🤔

Anyway I did a grep -rin "mip" . from the yt-fork root directory and replaced mips with maxs and think that looks pretty good now. The IsolatedGalaxy example does not look great for the maximum method. It also looks rather artefact-y for the minimum method. So I'll test on the GalaxyClusterMerge dataset later (need to re-download it 😬) that'll also provide a closer comparison to the existing simple projection cookbooks.

neutrinoceros · 2022-05-26T14:12:01Z

ok we got the docs build back up. I see that some tests are failing already but let's see what else is broken so we can iterate further.

The IsolatedGalaxy example does not look great for the maximum method. It also looks rather artefact-y for the minimum method.

I can't be the judge of that, but I do want to stress that with IsolatedGalaxy, the recipes takes about 10s, whereas with your preferred dataset it takes about a minute. This is about a 2% overhead on our docs build time, it's far from negligible, which is why I would like if we could find a better compromise.

rjfarber · 2022-05-26T14:16:33Z

I can't be the judge of that, but I do want to stress that with IsolatedGalaxy, the recipes takes about 10s, whereas with your preferred dataset it takes about a minute. This is about a 2% overhead on our docs build time, it's far from negligible, which is why I would like if we could find a better compromise.

I think this is a bit unfair given that the pre-existing most similar cookbook scripts simple_projection.py AND simple_projection_weighted.py both use that same dataset. That's the only reason I started with it in the first place.

neutrinoceros · 2022-05-26T14:24:31Z

That's fine, I can get behind that, and we can always optimise this later if we actually need to.

neutrinoceros · 2022-05-26T14:47:33Z

Got a couple failures on CI. Please run the failing tests locally and fix them. Do not hesitate if you have questions or need further help !

rjfarber · 2022-05-27T07:46:53Z

Okay! Need to do some research this morning but will plan to return to this in ~8 hours :) Best, -------- Ryan

…

On Thu, May 26, 2022 at 4:47 PM Clément Robert ***@***.***> wrote: Got a couple failures on CI. Please run the failing tests locally and fix them. Do not hesitate if you have questions or need further help ! — Reply to this email directly, view it on GitHub <#3940 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGNJQV4NE5AWDWSS3JXSWATVL6FI7ANCNFSM5W5ZTDIQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

neutrinoceros

I took a look at failures, here are some pointers for how to solve them

yt/visualization/tests/test_plotwindow.py

yt/data_objects/tests/test_numpy_ops.py

yt/data_objects/data_containers.py

yt/data_objects/tests/test_projection.py

Co-authored-by: Clément Robert <cr52@protonmail.com>

rjfarber · 2022-05-27T18:27:31Z

Okay I looked at the failures and think maybe they'll be good now. It looks like the checks are running above so will see if there's a new error to look at.

neutrinoceros · 2022-05-27T18:29:52Z

Awesome, thank you !
When everything passes I will add a final commit to make sure deprecation warnings are correctly implemented, then I'll merge.

neutrinoceros · 2022-05-27T19:04:14Z

@rjfarber one last failure, and it's coming from a test I "helped" with. I'm sorry if I broke it.

welcome · 2022-05-27T21:32:52Z

Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆

Ryan Farber and others added 2 commits May 25, 2022 18:27

Projection 'min' method (first commit, doesn't work yet). Address Iss…

898b5eb

…ue #987654321

added mention of the new projection method in 'simple_plots.rst' in t…

c600581

…he cookbook and a mwe as 'simple_projection_methods.py' - and it looks like it's working! At least partially :) Addresses Issue #987654321

neutrinoceros reviewed May 25, 2022

View reviewed changes

doc/source/cookbook/simple_projection_methods.py Outdated Show resolved Hide resolved

switched back to the standard file to load in the cookbook script

f6d0d3c

[pre-commit.ci] auto fixes from pre-commit.com hooks

68f29a9

for more information, see https://pre-commit.ci

neutrinoceros added the new feature Something fun and new! label May 25, 2022

matthewturk changed the title ~~Adds "Min" Method to ProjectionPlot~~ ENH: Adds "Min" Method to ProjectionPlot May 25, 2022

neutrinoceros reviewed May 25, 2022

View reviewed changes

rjfarber added 2 commits May 26, 2022 10:31

changed 'in' to '==' and separated save from instantiation in cookboo…

b64d182

…k. Address Issue #987654321

added deprecation warning for 'mip' method value and removed usage of…

5931042

… 'mip' in construction_data_containers. I'm not sure now if I should've done that to clean or left it in there to be safe...

neutrinoceros reviewed May 26, 2022

View reviewed changes

yt/data_objects/construction_data_containers.py Outdated Show resolved Hide resolved

corrected the deprecation warning to actually have the 'if' statement…

1b7851f

… it was intended to have. Addresses issue #987654321

neutrinoceros requested changes May 26, 2022

View reviewed changes

doc/source/cookbook/simple_projection_methods.py Outdated Show resolved Hide resolved

doc/source/cookbook/simple_projection_methods.py Show resolved Hide resolved

Update doc/source/cookbook/simple_projection_methods.py

e5aa195

custom file name Co-authored-by: Clément Robert <cr52@protonmail.com>

neutrinoceros reviewed May 26, 2022

View reviewed changes

yt/data_objects/construction_data_containers.py Outdated Show resolved Hide resolved

neutrinoceros reviewed May 26, 2022

View reviewed changes

rjfarber added 9 commits May 26, 2022 13:54

added max,min to docstring in plot_window.py; note I'll be doing a lo…

7ca0939

…t of little commits since I made a lot of changes and just lost them all :*( Addresses Issue #987654321

just ran pre-commit (had to reinstall)

9090462

if/elif fixed (again...)

3301312

added min,max to test_plotwindow. Left mip in...

e515118

changed mip to max in data_containers max method, both docstring and …

b67ea67

…projection method call

replaced 'mip' with 'max' in test_projection.py

f1852d3

added 'min' tests and replaced 'mip' with 'max'

7bbeef9

changed 'mip' to 'max' in test_minimal_representation.py

1e0bc9c

replace 'mip' with 'max' in cookbook/image_resolution.py

7f39367

rjfarber added 4 commits May 26, 2022 14:41

incorporating black-jupyter fixes

88face7

not implemented error msg added

dadf8fe

added msg to not implemented error...not as sure about this one

b6b1d19

precommit fixes

893d9f2

removed msgs from not implemented errors in yt/visualization scripts

02a667d

neutrinoceros added the deprecation deprecate features or remove deprecated ones label May 26, 2022

neutrinoceros reviewed May 27, 2022

View reviewed changes

yt/visualization/tests/test_plotwindow.py Outdated Show resolved Hide resolved

yt/data_objects/tests/test_numpy_ops.py Show resolved Hide resolved

neutrinoceros reviewed May 27, 2022

View reviewed changes

yt/data_objects/data_containers.py Outdated Show resolved Hide resolved

neutrinoceros reviewed May 27, 2022

View reviewed changes

yt/data_objects/tests/test_projection.py Outdated Show resolved Hide resolved

rjfarber and others added 5 commits May 27, 2022 20:12

Update yt/visualization/tests/test_plotwindow.py

d489942

Co-authored-by: Clément Robert <cr52@protonmail.com>

Update yt/data_objects/data_containers.py

e848ae2

Co-authored-by: Clément Robert <cr52@protonmail.com>

Update yt/data_objects/tests/test_projection.py

21344cb

Co-authored-by: Clément Robert <cr52@protonmail.com>

added assert in test_projection

06ecc8b

added min implementation

265c9a8

neutrinoceros added 3 commits May 27, 2022 21:37

fix broken test

9d6dabb

DEPR: add missing deprecation warning

0060e05

cleanup

0eec914

neutrinoceros approved these changes May 27, 2022

View reviewed changes

neutrinoceros merged commit 0fbc997 into yt-project:main May 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Adds "Min" Method to ProjectionPlot #3940

ENH: Adds "Min" Method to ProjectionPlot #3940

rjfarber commented May 25, 2022

welcome bot commented May 25, 2022

matthewturk commented May 25, 2022

neutrinoceros left a comment

matthewturk commented May 25, 2022

matthewturk commented May 25, 2022

neutrinoceros left a comment

neutrinoceros May 26, 2022

neutrinoceros May 26, 2022

rjfarber May 26, 2022

neutrinoceros May 26, 2022

rjfarber May 26, 2022

neutrinoceros May 26, 2022

rjfarber May 26, 2022

neutrinoceros May 26, 2022

rjfarber May 26, 2022

neutrinoceros May 27, 2022

neutrinoceros left a comment

rjfarber commented May 26, 2022

neutrinoceros commented May 26, 2022

rjfarber commented May 26, 2022

neutrinoceros commented May 26, 2022

neutrinoceros commented May 26, 2022

rjfarber commented May 27, 2022 via email

neutrinoceros left a comment

rjfarber commented May 27, 2022

neutrinoceros commented May 27, 2022

neutrinoceros commented May 27, 2022

welcome bot commented May 27, 2022

ENH: Adds "Min" Method to ProjectionPlot #3940

ENH: Adds "Min" Method to ProjectionPlot #3940

Conversation

rjfarber commented May 25, 2022

PR Summary

PR Checklist

welcome bot commented May 25, 2022

matthewturk commented May 25, 2022

neutrinoceros left a comment

Choose a reason for hiding this comment

matthewturk commented May 25, 2022

matthewturk commented May 25, 2022

neutrinoceros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neutrinoceros left a comment

Choose a reason for hiding this comment

rjfarber commented May 26, 2022

neutrinoceros commented May 26, 2022

rjfarber commented May 26, 2022

neutrinoceros commented May 26, 2022

neutrinoceros commented May 26, 2022

rjfarber commented May 27, 2022 via email

neutrinoceros left a comment

Choose a reason for hiding this comment

rjfarber commented May 27, 2022

neutrinoceros commented May 27, 2022

neutrinoceros commented May 27, 2022

welcome bot commented May 27, 2022